Make Blob.FromStream GC safe by jeremy-visionaid · Pull Request #3473 · mono/SkiaSharp

jeremy-visionaid · 2026-01-28T21:56:52Z

Description of Change

Avoids a potential crash from memory being moved by GC

Bugs Fixed

Fixes [BUG] Blob FromStream is not GC safe #3472

API Changes

None.

Behavioral Changes

None.

Required skia PR

None.

PR Checklist

Has tests (if omitted, state reason in description)
Rebased on top of main at time of PR
Merged related skia PRs
Changes adhere to coding standard
Updated documentation

dotnet-policy-service · 2026-01-28T21:57:14Z

Hey there @@jeremy-visionaid! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

Fixes: mono#3472

Copilot

Pull request overview

This pull request fixes a critical GC safety bug in Blob.FromStream where managed array memory could be moved by the garbage collector after the blob was created, potentially causing crashes. The fix replaces the managed memory approach (using a byte array with fixed) with unmanaged memory allocation using Marshal.AllocCoTaskMem, ensuring the memory location remains stable throughout the blob's lifetime.

Changes:

Replaced managed memory pattern (MemoryStream + ToArray + fixed) with unmanaged memory allocation pattern (Marshal.AllocCoTaskMem + UnmanagedMemoryStream)
Updated the existing test to use separate using statements for better readability
Added a new GC safety test that explicitly verifies the blob data remains accessible after garbage collection

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.

File	Description
binding/HarfBuzzSharp/Blob.cs	Replaced GC-unsafe managed memory with unmanaged memory allocation using Marshal.AllocCoTaskMem/FreeCoTaskMem
tests/Tests/HarfBuzzSharp/HBBlobTest.cs	Refactored existing test and added new GC safety test that forces collection and verifies data access

mattleibow

PR Review: Make Blob.FromStream GC Safe (#3473)

Hey @jeremy-visionaid! Thanks for catching this GC safety issue — it's a subtle but serious bug that could cause random crashes. The core fix (using unmanaged memory instead of a pinned managed array) is the right approach. 👍

I have a couple of concerns before this can be merged:

🔴 Non-seekable streams will throw

The new implementation accesses stream.Length and stream.Position:

var length = (int)(stream.Length - stream.Position);

This throws NotSupportedException on non-seekable streams like NetworkStream, GZipStream, or HTTP response streams. The original implementation handled these correctly by using MemoryStream.CopyTo() which reads until EOF.

Suggested fix:

// For non-seekable streams, buffer into memory first
if (!stream.CanSeek)
{
    using var ms = new MemoryStream ();
    stream.CopyTo (ms);
    ms.Position = 0;
    return FromStream (ms);
}

🔴 Memory leak if an exception occurs

If stream.CopyTo(ums) throws, the allocated memory is never freed:

var dataPtr = Marshal.AllocCoTaskMem (length);
// If CopyTo throws, dataPtr leaks
using var ums = new UnmanagedMemoryStream ((byte*)dataPtr, length, length, FileAccess.ReadWrite);
stream.CopyTo (ums);

Suggested fix: Wrap in try/catch:

var dataPtr = Marshal.AllocCoTaskMem (length);
try
{
    using var ums = new UnmanagedMemoryStream ((byte*)dataPtr, length, length, FileAccess.ReadWrite);
    stream.CopyTo (ums);
    return new Blob (dataPtr, length, MemoryMode.ReadOnly, () => Marshal.FreeCoTaskMem (dataPtr));
}
catch
{
    Marshal.FreeCoTaskMem (dataPtr);
    throw;
}

🟡 Test suggestions

A couple of additional test cases would help ensure edge cases are covered:

Non-seekable stream test:

[SkippableFact]
public void ShouldCreateFromNonSeekableStream()
{
    using var fileStream = File.Open(Path.Combine(PathToFonts, "Funkster.ttf"), FileMode.Open, FileAccess.Read);
    using var nonSeekable = new NonSeekableReadOnlyStream(fileStream);
    using var blob = Blob.FromStream(nonSeekable);
    Assert.Equal(236808, blob.Length);
}

Partially-read stream test:

[SkippableFact]
public void ShouldCreateFromPartiallyReadStream()
{
    using var stream = File.Open(Path.Combine(PathToFonts, "Funkster.ttf"), FileMode.Open, FileAccess.Read);
    
    // Read first 100 bytes, simulating a partially consumed stream
    var header = new byte[100];
    stream.Read(header, 0, 100);
    
    using var blob = Blob.FromStream(stream);
    Assert.Equal(236808 - 100, blob.Length);
}

Complete suggested implementation

Putting it all together:

public static unsafe Blob FromStream (Stream stream)
{
    if (stream == null)
        throw new ArgumentNullException (nameof (stream));

    // For non-seekable streams, buffer into memory first
    if (!stream.CanSeek)
    {
        using var ms = new MemoryStream ();
        stream.CopyTo (ms);
        ms.Position = 0;
        return FromStream (ms);
    }

    var length = (int)(stream.Length - stream.Position);
    if (length == 0)
        return Empty;

    var dataPtr = Marshal.AllocCoTaskMem (length);
    try
    {
        using var ums = new UnmanagedMemoryStream ((byte*)dataPtr, length, length, FileAccess.ReadWrite);
        stream.CopyTo (ums);
        return new Blob (dataPtr, length, MemoryMode.ReadOnly, () => Marshal.FreeCoTaskMem (dataPtr));
    }
    catch
    {
        Marshal.FreeCoTaskMem (dataPtr);
        throw;
    }
}

Thanks again for the contribution! Let me know if you have any questions about the suggested changes.

Expand the skill's scan taxonomy from 6 to 12 families, each backed by a real historical SkiaSharp leak fix (finalizer/collection ordering #3796/#3291, Clone double-free #2904, disposing native statics #1863/#4080/#1224, field-not-nulled #1256/#1344, stream/callback/delegate-proxy lifetime #3589/#2916/#996, allocation-failure #1784/#1642). Broaden Phase 1.3 de-dup to search by api/type name (real leaks are filed as [BUG], not [memory-leak]) and add the Blob.FromStream / PR #3473 worked example. Extend the Phase 3.2 fix table to cover all 12 families. Document two verified, un-filed family-6 candidates surfaced by a model-diverse scan (SKRegion.SpanIterator missing parent ref; SKPixmap.ExtractSubset/With* not propagating pixelSource). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Audit of all 11 families for over-specific/live targets that would let the agent copy an answer instead of investigating: - Family 4 (fixed-pointer): replace the "real, still-unfixed Blob.FromStream" worked example with a synthetic non-copying-native example; demote Blob.FromStream to a bare #3472/PR #3473 citation. Same for the SKILL.md cheat-sheet row 4. - Family 3 (Views): drop the embedded live SKGLElement / open-PR #3311 diagnosis from "Real cases"; keep the issue numbers only. - Family 6 (clone double-free): genericize the SKPaint.Clone example to a neutral SKThing/sk_thing_clone wrapper; keep #2904 citation. Remaining real symbols are all safe: documented same-instance contracts (family 2), already-fixed issue citations, or correct reference implementations to mirror (families 7-10). No un-fixed target is handed to the agent as a worked answer. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

github-project-automation Bot added this to SkiaSharp Backlog Jan 28, 2026

dotnet-policy-service Bot added the community ✨ label Jan 28, 2026

Make Blob.FromStream GC safe

e7b3d4d

Fixes: mono#3472

jeremy-visionaid force-pushed the make-blob-from-stream-gc-safe branch from 986d4d3 to e7b3d4d Compare January 28, 2026 22:05

mattleibow requested a review from Copilot January 29, 2026 01:00

Copilot started reviewing on behalf of mattleibow January 29, 2026 01:00 View session

Copilot AI reviewed Jan 29, 2026

View reviewed changes

mattleibow requested changes Jan 29, 2026

View reviewed changes

github-project-automation Bot moved this to Changes Requested in SkiaSharp Backlog Jan 29, 2026

jeremy-visionaid added 2 commits January 29, 2026 14:32

Handle non-seekable streams in Blob.FromStream

085f2ee

Improve CreateFromStreamIsGCSafe

484b017

jeremy-visionaid requested a review from mattleibow January 29, 2026 01:49

mattleibow added a commit that referenced this pull request Feb 13, 2026

ai-triage: classify #3473

7f2fb93

github-actions Bot mentioned this pull request Apr 17, 2026

Daily Digest –2026-04-17 #3695

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Make Blob.FromStream GC safe#3473

Make Blob.FromStream GC safe#3473
jeremy-visionaid wants to merge 3 commits into
mono:mainfrom
jeremy-visionaid:make-blob-from-stream-gc-safe

jeremy-visionaid commented Jan 28, 2026

Uh oh!

dotnet-policy-service Bot commented Jan 28, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattleibow left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

jeremy-visionaid commented Jan 28, 2026

Uh oh!

dotnet-policy-service Bot commented Jan 28, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattleibow left a comment

Choose a reason for hiding this comment

PR Review: Make Blob.FromStream GC Safe (#3473)

🔴 Non-seekable streams will throw

🔴 Memory leak if an exception occurs

🟡 Test suggestions

Complete suggested implementation

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants